feat: ESM resolver hardening, http loader, dev-mode globals#383
feat: ESM resolver hardening, http loader, dev-mode globals#383NathanWalker wants to merge 15 commits into
Conversation
📝 WalkthroughWalkthroughThis PR adds an HTTP-based ESM module loading and HMR dev-session system to the NativeScript iOS runtime, including per-isolate module registries, import-map support, a blob URL polyfill, and worker termination. It also upgrades CI to macOS 15/Xcode 26 with diagnostics collection, hardens the Embassy test HTTP server, and adds quarantine/progress tracking plus expanded test coverage. ChangesRuntime: HMR & HTTP module system
CI, test harness & test coverage
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Runtime
participant HMRSupport
participant ModuleInternal
participant Worker
Runtime->>HMRSupport: InitializeHmrDevGlobals(isolate, context, isWorker)
HMRSupport->>HMRSupport: KickstartHmrPrefetchUrlsSync, SetDevBootComplete
Runtime->>ModuleInternal: RunModule(path, outErrorMessage)
ModuleInternal->>ModuleInternal: LoadHttpModuleForUrl / LoadESModule
ModuleInternal-->>Runtime: bool + optional error
Runtime->>HMRSupport: CleanupHMRGlobals (main isolate only)
Runtime->>Worker: __NS_DEV__.terminateAllWorkers
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
8310eac to
2c5d877
Compare
6dfbacd to
f7cdfcc
Compare
|
I'm not sure I understand the goal of these "dev sessions"? What do these need specifically from the runtime that's not an "user land" thing? |
Yeah good question @edusperoni, the "dev session" naming here probably over (or mis) characterizes things. The "session" part is just the contract for booting from a dev server over HTTP instead of the on-disk bundle. It does three things a one-shot bundle loader doesnt: point resolution at an HTTP origin + install the import map before the first import, give a re-entrant boot (import client > import entry) that can re-run for a full reload without relaunching the process, and bubble import failures back as a rejected promise so the client can show an overlay instead of the app just dying. Could it be userland? I think the thing that trips people up (tripped me up too) is that on the web HMR is userland because the browser is the runtime; it already ships a spec ESM loader that fetches over HTTP and a host-owned module map you poke at by varying the URL. Vite's client gets to be "just JS" because it sits on top of that. Here V8 is embedded by us, and bare V8 ships no loader at all; every piece of it is an embedder host callback only native can install. So the litmus test is pretty clean: anything that has to install/drive a V8 host callback or mutate V8's module map cant be userland, everything else stays in JS. This may help expand a few things:
So really these globals arent "a dev-session feature" so much as the embedder half of a spec ESM loader + an identity-preserving module map. The part the browser hands Vite for free. There is likely a few that could move to JS if we want a smaller surface like __nsApplyStyleUpdate (just Application.addCss + restyle) and __nsGetLoadedModuleUrls (introspection) and others. Lmk if you see the boundary differently and we can make further adjustments. |
d24c897 to
4289539
Compare
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
NativeScript/runtime/Runtime.mm (1)
445-458: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor the new
RunModulefailure contract in main startup.Line 450 still discards the boolean result. Since
ModuleInternal::RunModulenow reports some failures by returningfalsewithout throwing, startup can continue after the main module failed.Proposed fix
void Runtime::RunMainScript() { Isolate* isolate = this->GetIsolate(); v8::Locker locker(isolate); Isolate::Scope isolate_scope(isolate); HandleScope handle_scope(isolate); - this->moduleInternal_->RunModule(isolate, "./"); + std::string err; + if (!this->moduleInternal_->RunModule(isolate, "./", &err)) { + throw NativeScriptException( + isolate, + err.empty() ? "Failed to run main module" : err, + "Error"); + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/Runtime.mm` around lines 445 - 458, Runtime::RunMainScript currently ignores the boolean result from ModuleInternal::RunModule, so startup can continue even when main module loading fails without throwing. Update RunMainScript to use the same failure contract as Runtime::RunModule by capturing the return value from moduleInternal_->RunModule and handling a false result as a startup failure, using the existing Runtime and ModuleInternal::RunModule symbols to locate the change.
🧹 Nitpick comments (4)
NativeScript/runtime/ModuleInternalCallbacks.h (1)
44-45: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winKeep loaded-module introspection isolate-explicit.
Now that the registry is keyed by
v8::Isolate*,GetLoadedModuleUrls()should take the target isolate like the other registry APIs. Relying on an implicit current isolate makes worker/main diagnostics easier to mix up.Suggested API adjustment
-std::vector<std::string> GetLoadedModuleUrls(); +std::vector<std::string> GetLoadedModuleUrls(v8::Isolate* isolate);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/ModuleInternalCallbacks.h` around lines 44 - 45, GetLoadedModuleUrls() is still using an implicit current isolate, which can mix up worker and main-thread diagnostics now that the module registry is isolate-keyed. Update the ModuleInternalCallbacks API so GetLoadedModuleUrls takes a v8::Isolate* parameter, and propagate that isolate through the implementation and any call sites to match the other registry helpers. Use the existing registry symbols in ModuleInternalCallbacks to keep the diagnostics explicitly scoped to the target isolate..github/workflows/npm_release.yml (1)
263-279: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the failure diagnostics payload.
Copying the full CoreSimulator log tree plus an unrestricted
log collectcan make failed CI runs slow or produce oversized artifacts. Prefer a targeted logarchive window and avoid uploading the whole CoreSimulator directory.Suggested tightening
# Simulator app crashes land in the host's DiagnosticReports. cp -R ~/Library/Logs/DiagnosticReports/. "$DIAG/DiagnosticReports/" 2>/dev/null || true - cp -R ~/Library/Logs/CoreSimulator/. "$DIAG/CoreSimulator/" 2>/dev/null || true + # Avoid uploading the full CoreSimulator log tree; the targeted + # logarchive below contains the simulator logs needed for this run. @@ - xcrun simctl spawn "$UDID" log collect --output "$DIAG/simulator.logarchive" 2>/dev/null || true + xcrun simctl spawn "$UDID" log collect --last 45m --output "$DIAG/simulator.logarchive" 2>/dev/null || truePlease verify the
log collect --lastoption on the macOS 15/Xcode 26 runner image.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/npm_release.yml around lines 263 - 279, The failure diagnostics step is too broad: it copies the entire CoreSimulator log tree and runs an unbounded log collect, which can create oversized artifacts and slow CI. In the diagnostics block that uses DIAG, xcrun simctl spawn, and log collect, stop archiving the full CoreSimulator directory and switch to a targeted unified-log collection window using the macOS 15/Xcode 26-supported log collect --last option so only recent logs are captured.NativeScript/runtime/Runtime.mm (1)
252-253: 🩺 Stability & Availability | 🔵 TrivialTrack the worker queue race TODO.
This TODO names a possible worker queue leak during termination ordering. Please track it before merge or file a follow-up so it does not get lost. I can help draft the issue or a fix plan.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/Runtime.mm` around lines 252 - 253, The TODO in Runtime.mm about a possible worker queue leak/race during termination ordering needs to be tracked before merge. Follow up on the worker lifecycle path in the Runtime-related initialization/termination flow, especially around the queue handling and the Terminate-before-Initialize scenario, and either replace the TODO with a concrete fix or create a tracked issue/fix plan linked to the worker queue race so it is not lost.NativeScript/runtime/URLImpl.cpp (1)
59-90: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKeep
searchParamssynchronized afterurl.searchchanges.The getter returns the cached
_searchParamsforever. If code readsurl.searchParams, then later assignsurl.search, subsequenturl.searchParamsreads still expose the old query.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/URLImpl.cpp` around lines 59 - 90, The URLImpl::searchParams getter caches a single _searchParams instance and never refreshes it when url.search changes, so later reads can return stale query data. Update the URL.prototype.searchParams handling in URLImpl.cpp so the cached URLSearchParams is invalidated or resynced whenever the search setter runs, and make sure the getter recreates/updates the instance from the current search string before returning it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@NativeScript/runtime/DevFlags.mm`:
- Around line 95-103: The allowlist check in RemoteUrlMatchesAllowlistEntry is
matching raw URL prefixes too early, which lets path-scoped entries be bypassed
with dot-segment paths. Update the matching logic to canonicalize or normalize
the URL path before applying the prefix/boundary rules, or explicitly reject
plain/encoded dot segments in DevFlags.mm so a trailing-slash allowlist entry
cannot match escaped paths.
In `@NativeScript/runtime/ModuleInternal.mm`:
- Around line 236-264: The debug handling in ModuleInternal.mm is swallowing
non-HTTP ES module failures by returning true or an empty namespace, which
prevents worker error propagation for .mjs loads. Update the
NativeScriptException catch and the moduleNamespace.IsEmpty path so debug mode
still surfaces failures to the caller for worker-loaded ESM, instead of always
pretending success; keep the existing HTTP debug logging, but ensure the
Worker.mm TryCatch can observe the error for the ES module load path.
In `@NativeScript/runtime/URLImpl.cpp`:
- Around line 94-100: The install script execution in URLImpl should not fail
silently: the current Compile/Run flow can leave blob URL support partially
initialized and a pending V8 exception uncleared. Update the script path in
URLImpl to wrap the v8::Script::Compile and script->Run calls in a v8::TryCatch,
then explicitly handle both compile and runtime failures by logging the error
and propagating it (or throwing) instead of ignoring the result. Use the
existing blob_methods script setup in URLImpl as the place to add this error
handling.
In `@NativeScript/runtime/Worker.mm`:
- Around line 486-499: The HMR termination loop in Worker::TerminateWorkers
currently iterates all entries from Caches::Workers, which can affect workers
from other runtimes. Update this callback to filter worker wrappers by the
current main isolate, matching the existing Runtime::~Runtime() behavior via
WorkerWrapper::GetMainIsolate(). Keep the existing running/closing checks and
only call WorkerWrapper::Terminate() for workers belonging to the same isolate.
In `@TestRunner/app/tests/esm/hmr/hot-data-ext.js`:
- Around line 51-73: The hot-data fixture is mutating shared HMR state by
invoking hot.accept, hot.dispose, hot.decline, and hot.invalidate in the shared
test helper, which makes later specs order-dependent. Update hot-data-ext.js so
the shared fixture only checks for the presence of HMR APIs and data on hot, and
remove lifecycle/callback registration from this path. If coverage for those
methods is needed, move it into a separate throwaway module or dedicated test
helper that is not reused across specs.
In `@TestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjs`:
- Around line 55-78: The test cleanup in the __ns_test_vendor__ import-map spec
is not restoring the runtime’s prior import-map state, which can leak
configuration into later specs. Snapshot the existing import-map before calling
configureRuntime in this test, then in the finally block restore that original
import-map instead of resetting to an empty imports object. Keep the existing
__nsVendorRegistry restore logic intact so the test remains hermetic.
In `@TestRunnerTests/Embassy/TCPSocket.swift`:
- Around line 60-66: The SO_NOSIGPIPE setup in TCPSocket is currently ignoring
the result of setsockopt, which can leave the socket in a state where send may
trigger SIGPIPE before Transport.handleWrite() can observe EPIPE. Update the
socket setup path in the TCPSocket initializer/helper to check the setsockopt
return value, and if it fails on Darwin, immediately treat it as a socket error
by closing the socket and propagating an error back to the caller instead of
discarding it. Use the TCPSocket and Transport.handleWrite symbols to locate the
write-path setup and keep the failure handling aligned with the existing socket
lifecycle.
In `@TestRunnerTests/TestRunnerTests.swift`:
- Line 24: The test setup in DefaultHTTPServer is still binding the server to
127.0.0.1 even though TCPSocket.bind(interface:) currently treats the interface
as IPv6, so the listener is not truly IPv4. Update the TestRunnerTests/server
setup to use the matching loopback family consistently (for example, switch both
server and client-side expectations to ::1/[::1]), or if IPv4 is required,
adjust TCPSocket and the DefaultHTTPServer path to support AF_INET first. Use
the existing DefaultHTTPServer initializer and TCPSocket.bind interface handling
to locate the change.
---
Outside diff comments:
In `@NativeScript/runtime/Runtime.mm`:
- Around line 445-458: Runtime::RunMainScript currently ignores the boolean
result from ModuleInternal::RunModule, so startup can continue even when main
module loading fails without throwing. Update RunMainScript to use the same
failure contract as Runtime::RunModule by capturing the return value from
moduleInternal_->RunModule and handling a false result as a startup failure,
using the existing Runtime and ModuleInternal::RunModule symbols to locate the
change.
---
Nitpick comments:
In @.github/workflows/npm_release.yml:
- Around line 263-279: The failure diagnostics step is too broad: it copies the
entire CoreSimulator log tree and runs an unbounded log collect, which can
create oversized artifacts and slow CI. In the diagnostics block that uses DIAG,
xcrun simctl spawn, and log collect, stop archiving the full CoreSimulator
directory and switch to a targeted unified-log collection window using the macOS
15/Xcode 26-supported log collect --last option so only recent logs are
captured.
In `@NativeScript/runtime/ModuleInternalCallbacks.h`:
- Around line 44-45: GetLoadedModuleUrls() is still using an implicit current
isolate, which can mix up worker and main-thread diagnostics now that the module
registry is isolate-keyed. Update the ModuleInternalCallbacks API so
GetLoadedModuleUrls takes a v8::Isolate* parameter, and propagate that isolate
through the implementation and any call sites to match the other registry
helpers. Use the existing registry symbols in ModuleInternalCallbacks to keep
the diagnostics explicitly scoped to the target isolate.
In `@NativeScript/runtime/Runtime.mm`:
- Around line 252-253: The TODO in Runtime.mm about a possible worker queue
leak/race during termination ordering needs to be tracked before merge. Follow
up on the worker lifecycle path in the Runtime-related
initialization/termination flow, especially around the queue handling and the
Terminate-before-Initialize scenario, and either replace the TODO with a
concrete fix or create a tracked issue/fix plan linked to the worker queue race
so it is not lost.
In `@NativeScript/runtime/URLImpl.cpp`:
- Around line 59-90: The URLImpl::searchParams getter caches a single
_searchParams instance and never refreshes it when url.search changes, so later
reads can return stale query data. Update the URL.prototype.searchParams
handling in URLImpl.cpp so the cached URLSearchParams is invalidated or resynced
whenever the search setter runs, and make sure the getter recreates/updates the
instance from the current search string before returning it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5096dcca-ed10-47ea-be7c-64aafa7275ac
📒 Files selected for processing (28)
.github/scripts/sample-hung-app.sh.github/workflows/npm_release.ymlNativeScript/runtime/DevFlags.hNativeScript/runtime/DevFlags.mmNativeScript/runtime/HMRSupport.hNativeScript/runtime/HMRSupport.mmNativeScript/runtime/ModuleInternal.hNativeScript/runtime/ModuleInternal.mmNativeScript/runtime/ModuleInternalCallbacks.hNativeScript/runtime/ModuleInternalCallbacks.mmNativeScript/runtime/Runtime.hNativeScript/runtime/Runtime.mmNativeScript/runtime/URLImpl.cppNativeScript/runtime/URLImpl.hNativeScript/runtime/Worker.hNativeScript/runtime/Worker.mmTestRunner/app/Infrastructure/Jasmine/jasmine-2.0.1/boot.jsTestRunner/app/tests/HttpEsmLoaderTests.jsTestRunner/app/tests/MethodCallsTests.jsTestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjsTestRunner/app/tests/RemoteModuleSecurityTests.jsTestRunner/app/tests/esm/hmr/hot-data-ext.jsTestRunner/app/tests/esm/hmr/hot-data-ext.mjsTestRunnerTests/Embassy/DefaultHTTPServer.swiftTestRunnerTests/Embassy/TCPSocket.swiftTestRunnerTests/Embassy/Transport.swiftTestRunnerTests/QUARANTINED_TESTS.mdTestRunnerTests/TestRunnerTests.swift
| static bool RemoteUrlMatchesAllowlistEntry(const std::string& url, | ||
| const std::string& entry) { | ||
| if (entry.empty()) return false; | ||
| if (url.size() < entry.size()) return false; | ||
| if (url.compare(0, entry.size(), entry) != 0) return false; | ||
| if (url.size() == entry.size()) return true; // exact match | ||
| if (entry.back() == '/') return true; // entry ended at a boundary | ||
| const char next = url[entry.size()]; | ||
| return next == '/' || next == '?' || next == '#'; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Normalize paths before matching path-scoped allowlist entries.
Line 101 authorizes any raw URL with a trailing-slash entry prefix, so an allowlist entry like https://cdn.example.com/app/ also matches https://cdn.example.com/app/../admin.js. If the fetch layer or server normalizes dot segments, this escapes the intended path scope. Parse/canonicalize the URL path before matching, or reject encoded/plain dot segments before applying the prefix rule.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@NativeScript/runtime/DevFlags.mm` around lines 95 - 103, The allowlist check
in RemoteUrlMatchesAllowlistEntry is matching raw URL prefixes too early, which
lets path-scoped entries be bypassed with dot-segment paths. Update the matching
logic to canonicalize or normalize the URL path before applying the
prefix/boundary rules, or explicitly reject plain/encoded dot segments in
DevFlags.mm so a trailing-slash allowlist entry cannot match escaped paths.
| v8::Local<v8::Script> script; | ||
| auto compiled = v8::Script::Compile(context, ToV8String(isolate, blob_methods)).ToLocal(&script); | ||
|
|
||
| if (compiled) { | ||
| v8::Local<v8::Value> outVal; | ||
| (void)script->Run(context).ToLocal(&outVal); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Handle install script failures explicitly.
If this script fails to compile or run, blob URL support is silently absent and a pending V8 exception may be left behind. Wrap it in TryCatch and log/throw the failure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@NativeScript/runtime/URLImpl.cpp` around lines 94 - 100, The install script
execution in URLImpl should not fail silently: the current Compile/Run flow can
leave blob URL support partially initialized and a pending V8 exception
uncleared. Update the script path in URLImpl to wrap the v8::Script::Compile and
script->Run calls in a v8::TryCatch, then explicitly handle both compile and
runtime failures by logging the error and propagating it (or throwing) instead
of ignoring the result. Use the existing blob_methods script setup in URLImpl as
the place to add this error handling.
| int32_t terminatedCount = 0; | ||
| for (auto& state : snapshot) { | ||
| auto* worker = static_cast<WorkerWrapper*>(state->UserData()); | ||
| if (worker == nullptr) { | ||
| continue; | ||
| } | ||
| if (!worker->IsRunning() || worker->IsClosing()) { | ||
| // Already torn down or in the process of closing. Counting these | ||
| // would inflate the diagnostic count returned to JS; skip. | ||
| continue; | ||
| } | ||
| try { | ||
| worker->Terminate(); | ||
| ++terminatedCount; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Filter termination to the current main isolate.
This callback terminates every worker in Caches::Workers. Runtime::~Runtime() already filters by WorkerWrapper::GetMainIsolate(), so this HMR helper should do the same to avoid one runtime/dev session killing another runtime’s workers.
Proposed fix
int32_t terminatedCount = 0;
+ Isolate* mainIsolate = info.GetIsolate();
for (auto& state : snapshot) {
auto* worker = static_cast<WorkerWrapper*>(state->UserData());
if (worker == nullptr) {
continue;
}
+ if (worker->GetMainIsolate() != mainIsolate) {
+ continue;
+ }
if (!worker->IsRunning() || worker->IsClosing()) {
continue;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int32_t terminatedCount = 0; | |
| for (auto& state : snapshot) { | |
| auto* worker = static_cast<WorkerWrapper*>(state->UserData()); | |
| if (worker == nullptr) { | |
| continue; | |
| } | |
| if (!worker->IsRunning() || worker->IsClosing()) { | |
| // Already torn down or in the process of closing. Counting these | |
| // would inflate the diagnostic count returned to JS; skip. | |
| continue; | |
| } | |
| try { | |
| worker->Terminate(); | |
| ++terminatedCount; | |
| int32_t terminatedCount = 0; | |
| Isolate* mainIsolate = info.GetIsolate(); | |
| for (auto& state : snapshot) { | |
| auto* worker = static_cast<WorkerWrapper*>(state->UserData()); | |
| if (worker == nullptr) { | |
| continue; | |
| } | |
| if (worker->GetMainIsolate() != mainIsolate) { | |
| continue; | |
| } | |
| if (!worker->IsRunning() || worker->IsClosing()) { | |
| // Already torn down or in the process of closing. Counting these | |
| // would inflate the diagnostic count returned to JS; skip. | |
| continue; | |
| } | |
| try { | |
| worker->Terminate(); | |
| +terminatedCount; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@NativeScript/runtime/Worker.mm` around lines 486 - 499, The HMR termination
loop in Worker::TerminateWorkers currently iterates all entries from
Caches::Workers, which can affect workers from other runtimes. Update this
callback to filter worker wrappers by the current main isolate, matching the
existing Runtime::~Runtime() behavior via WorkerWrapper::GetMainIsolate(). Keep
the existing running/closing checks and only call WorkerWrapper::Terminate() for
workers belonging to the same isolate.
| try { | ||
| configureRuntime({ | ||
| importMap: { | ||
| imports: { | ||
| __ns_test_vendor__: "ns-vendor://__ns_test_vendor__", | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| const mod = await import("__ns_test_vendor__"); | ||
| const modAgain = await import("__ns_test_vendor__"); | ||
|
|
||
| expect(mod).toBeDefined(); | ||
| expect(modAgain).toBe(mod); | ||
| expect(mod.default).toEqual({ source: "vendor-default" }); | ||
| expect(mod.namedValue).toBe(7); | ||
| expect(mod.makeValue()).toBe("vendor-named"); | ||
| } finally { | ||
| configureRuntime({ importMap: { imports: {} } }); | ||
| if (typeof previousRegistry === "undefined") { | ||
| delete globalThis.__nsVendorRegistry; | ||
| } else { | ||
| globalThis.__nsVendorRegistry = previousRegistry; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Restore the previous import-map state here.
This spec snapshots __nsVendorRegistry, but it does not snapshot the preexisting runtime import-map before calling configureRuntime(...). Resetting to imports: {} in finally can clobber earlier resolver state for later specs, and if the runtime merges config, it may not even remove the temporary mapping. Restore the prior import-map alongside the registry so the test stays hermetic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@TestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjs` around lines 55
- 78, The test cleanup in the __ns_test_vendor__ import-map spec is not
restoring the runtime’s prior import-map state, which can leak configuration
into later specs. Snapshot the existing import-map before calling
configureRuntime in this test, then in the finally block restore that original
import-map instead of resetting to an empty imports object. Keep the existing
__nsVendorRegistry restore logic intact so the test remains hermetic.
| _ = setsockopt( | ||
| fileDescriptor, | ||
| SOL_SOCKET, | ||
| SO_NOSIGPIPE, | ||
| &value, | ||
| socklen_t(MemoryLayout<Int32>.size) | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant files and inspect the changed socket wrapper plus transport usage.
git ls-files 'TestRunnerTests/Embassy/*' | sed -n '1,120p'
printf '\n--- TCPSocket.swift ---\n'
nl -ba TestRunnerTests/Embassy/TCPSocket.swift | sed -n '1,220p'
printf '\n--- Transport.swift ---\n'
nl -ba TestRunnerTests/Embassy/Transport.swift | sed -n '1,220p'Repository: NativeScript/ios
Length of output: 1497
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read only the relevant socket-related definitions/usages to understand failure handling.
rg -n "SO_NOSIGPIPE|SIGPIPE|ignoreSigPipe|send\\(" TestRunnerTests -SRepository: NativeScript/ios
Length of output: 1654
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check platform availability of SO_NOSIGPIPE semantics from local headers/docs if present.
python3 - <<'PY'
import os,glob,subprocess,sys,shlex
candidates = [
"/usr/include/sys/socket.h",
"/usr/include/x86_64-linux-gnu/sys/socket.h",
"/usr/local/include/sys/socket.h",
]
for p in candidates:
if os.path.exists(p):
print("FOUND", p)
try:
out = subprocess.check_output(["grep","-n","SO_NOSIGPIPE",p], text=True, stderr=subprocess.STDOUT)
print(out)
except subprocess.CalledProcessError as e:
print("no SO_NOSIGPIPE in", p)
PYRepository: NativeScript/ios
Length of output: 265
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact changed implementation and surrounding comments.
sed -n '1,120p' TestRunnerTests/Embassy/TCPSocket.swift
printf '\n---\n'
sed -n '1,120p' TestRunnerTests/Embassy/Transport.swift
printf '\n---\n'
sed -n '60,90p' TestRunnerTests/QUARANTINED_TESTS.mdRepository: NativeScript/ios
Length of output: 9189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for the specific error handling around send/write to see whether EPIPE is handled and where SIGPIPE would matter.
sed -n '170,230p' TestRunnerTests/Embassy/TCPSocket.swift
printf '\n---\n'
sed -n '160,240p' TestRunnerTests/Embassy/Transport.swiftRepository: NativeScript/ios
Length of output: 4693
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether ignoreSigPipe is used anywhere else, especially before send.
rg -n "ignoreSigPipe\\s*=\\s*true|ignoreSigPipe\\s*=\\s*false|SO_NOSIGPIPE|SIGPIPE" TestRunnerTests/Embassy -SRepository: NativeScript/ios
Length of output: 804
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact comments around the changed logic and the quarantine note.
sed -n '30,80p' TestRunnerTests/Embassy/TCPSocket.swift
printf '\n---\n'
sed -n '66,86p' TestRunnerTests/QUARANTINED_TESTS.mdRepository: NativeScript/ios
Length of output: 3520
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the write path that relies on send() errors being surfaced.
sed -n '170,235p' TestRunnerTests/Embassy/Transport.swiftRepository: NativeScript/ios
Length of output: 2235
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check for any global SIGPIPE handling or alternative socket flags in the harness.
rg -n "signal\\(SIGPIPE|SIG_IGN|MSG_NOSIGNAL|SO_NOSIGPIPE|ignoreSigPipe" TestRunnerTests -SRepository: NativeScript/ios
Length of output: 1121
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect whether the transport can avoid calling send() on sockets that failed SO_NOSIGPIPE setup.
python3 - <<'PY'
from pathlib import Path
p = Path("TestRunnerTests/Embassy/TCPSocket.swift")
text = p.read_text()
start = text.index("var ignoreSigPipe")
end = text.index("init(blocking:", start)
print(text[start:end])
PYRepository: NativeScript/ios
Length of output: 1119
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for mentions of the changed failure mode in repo notes to see if this is an accepted tradeoff.
rg -n "best-effort|assertionFailure|Executed 0 tests|SIGPIPE|SO_NOSIGPIPE" TestRunnerTests/QUARANTINED_TESTS.md TestRunnerTests/Embassy -SRepository: NativeScript/ios
Length of output: 1543
🌐 Web query:
Darwin SO_NOSIGPIPE send SIGPIPE EPIPE setsockopt failure behavior documentation
💡 Result:
On Darwin (macOS/iOS), the SO_NOSIGPIPE socket option is the standard mechanism to prevent a process from receiving a SIGPIPE signal when writing to a stream-oriented socket that has no reader [1][2][3]. When SO_NOSIGPIPE is enabled via setsockopt, the following behavior occurs: 1. Signal Suppression: The kernel does not generate a SIGPIPE signal when a write operation fails due to a broken connection (e.g., the peer has closed the socket) [3][4]. 2. Error Reporting: Instead of terminating the process with a signal, the write operation (e.g., send, write) will fail and return -1, with errno set to EPIPE [3][5]. Usage: To enable this, call setsockopt with the SO_NOSIGPIPE option on the socket file descriptor, typically immediately after creation [6]: int value = 1; if (setsockopt(sock, SOL_SOCKET, SO_NOSIGPIPE, &value, sizeof(value)) < 0) { // Handle failure (e.g., log error or close socket) } Failure Behavior: If the setsockopt call fails, the socket will remain in its default state, which is vulnerable to SIGPIPE [7]. Because SO_NOSIGPIPE is a socket-level option (not a per-call flag like MSG_NOSIGNAL available on Linux), it is common practice for robust applications to check the return value of setsockopt [8][7]. If it fails, the application should consider the socket untrusted or handle the potential SIGPIPE signal globally (e.g., via signal(SIGPIPE, SIG_IGN)) to prevent unexpected process termination [7][6]. While failure of this system call is rare on valid sockets, unchecked failures can leave a process susceptible to process termination if a peer disconnects [8][7].
Citations:
- 1: https://curl.se/mail/lib-2004-10/0079.html
- 2: Socket: Received signal SIGPIPE on Darwin (Mac OS X) nim-lang/Nim#1784
- 3: https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/setsockopt.2.html
- 4: https://leopard-adc.pepas.com/documentation/Darwin/Reference/ManPages/man2/setsockopt.2.html
- 5: http://www.newosxbook.com/src.php?file=%2Fbsd%2Fman%2Fman2%2Fgetsockopt.2&tree=xnu
- 6: https://stackoverflow.com/questions/19509348/sigpipe-osx-and-disconnected-sockets
- 7: unix: unchecked setsockopt(SO_NOSIGPIPE) in uv__socket libuv/libuv#5087
- 8: Inconsistent SO_NOSIGPIPE error handling across backends cppalliance/corosio#230
Handle SO_NOSIGPIPE setup failures as socket errors. If setsockopt fails on Darwin, the next send(..., 0) can still raise SIGPIPE and abort the runner before Transport.handleWrite() sees EPIPE. Close the socket or return an error instead of discarding the result.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@TestRunnerTests/Embassy/TCPSocket.swift` around lines 60 - 66, The
SO_NOSIGPIPE setup in TCPSocket is currently ignoring the result of setsockopt,
which can leave the socket in a state where send may trigger SIGPIPE before
Transport.handleWrite() can observe EPIPE. Update the socket setup path in the
TCPSocket initializer/helper to check the setsockopt return value, and if it
fails on Darwin, immediately treat it as a socket error by closing the socket
and propagating an error back to the caller instead of discarding it. Use the
TCPSocket and Transport.handleWrite symbols to locate the write-path setup and
keep the failure handling aligned with the existing socket lifecycle.
|
|
||
| loop = try! SelectorEventLoop(selector: try! KqueueSelector()) | ||
| self.server = DefaultHTTPServer(eventLoop: loop!, port: port) { | ||
| self.server = DefaultHTTPServer(eventLoop: loop!, interface: "127.0.0.1", port: port) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
127.0.0.1 does not match the current socket bind path
DefaultHTTPServer still routes through TCPSocket.bind(..., interface:), which parses the interface as IPv6. With that implementation, "127.0.0.1" does not create a real IPv4 listener here, so the harness still depends on dual-stack behavior. If this test needs IPv4 specifically, TCPSocket needs AF_INET support first; otherwise keep both sides on ::1 / [::1].
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@TestRunnerTests/TestRunnerTests.swift` at line 24, The test setup in
DefaultHTTPServer is still binding the server to 127.0.0.1 even though
TCPSocket.bind(interface:) currently treats the interface as IPv6, so the
listener is not truly IPv4. Update the TestRunnerTests/server setup to use the
matching loopback family consistently (for example, switch both server and
client-side expectations to ::1/[::1]), or if IPv4 is required, adjust TCPSocket
and the DefaultHTTPServer path to support AF_INET first. Use the existing
DefaultHTTPServer initializer and TCPSocket.bind interface handling to locate
the change.
Adds the Hot Module Replacement runtime layer plus the supporting ESM resolver hardening and dev-session globals that make hot reload viable on iOS.
* `import.meta.hot`: `data`, `accept`, `dispose`, `prune`,
`decline`, `invalidate`, `on`/`off`/`send` event surface.
* Dev-session globals (`__nsStartDevSession`, `__nsReloadDevApp`,
`__nsInvalidateModules`, `__nsRunHmrDispose`, `__nsRunHmrPrune`,
`__nsKickstartHmrPrefetch`, `__nsGetLoadedModuleUrls`,
`__nsApplyStyleUpdate`, `__nsConfigureDevRuntime`,
`__nsTerminateAllWorkers`).
* Speculative HTTP module prefetch with canonical-key normalization so
`__ns_hmr__/v<N>` and `__ns_boot__/b<N>` tag prefixes share `hot.data`
identity across reload cycles.
* ESM resolver hardening in `ModuleInternalCallbacks.mm` to:
- Preserve synthetic-namespace identity (`ns-vendor://`,
`optional:`, `node:`, `blob:`) — these are NOT filesystem paths.
- Handle HTTP/HTTPS module URLs end-to-end (resolution, fetch,
canonical-key collapse, dynamic import).
- Compile `.json` imports into synthetic ES modules.
* `NodeBuiltinsAndOptionalModulesTests.mjs`, `HttpEsmLoaderTests.js`,
`hot-data-ext.{js,mjs}` test fixtures, plus integration wiring in
`TestRunnerTests.swift` and the Jasmine boot harness.
[skip ci]
[skip ci]
…XCTest HTTP server [skip ci]
…opAndWait [skip ci]
In debug builds the module loader swallows compile/require errors (CompileScript returns an empty script; RunModule logs and returns success) so a bad HMR edit doesn't abort the main app. That also swallowed a *worker's* entry-script error, so `worker.onerror` never fired (e.g. a worker loaded from a syntactically invalid script hung the spec until the Jasmine async timeout). Gate the debug swallow on `!isWorker`: worker isolates now propagate the error (as release already does) and keep the V8 exception pending so the worker entry's TryCatch routes it to `worker.onerror`. Main-isolate HMR behavior is unchanged.
…harden test server Quarantine (harness-level specFilter, no submodule edit; see TestRunnerTests/QUARANTINED_TESTS.md): - "HMR hot.data" + "URL Key Canonicalization" (8 specs): the in-runner Embassy test server can't answer the runtime's synchronous NSURLConnection GET (getPeerName EINVAL / no response delivered). The HMR loader itself works; this is a test-harness limitation, documented for re-enable. Test-server robustness (kept; also pre-stages the un-quarantine): - DefaultHTTPServer.handleNewConnection: tolerate getPeerName() failure and serve with a placeholder peer instead of crashing (fixes the DefaultHTTPServer.swift:87 EXC_BREAKPOINT) or dropping the connection. - /esm/timeout.mjs: respond via non-blocking loop.call(withDelay:) instead of Thread.sleep, which wedged the single-threaded event loop. - Serve the /ns/m/... hot-data aliases and /ns/core bridge endpoints. [skip ci]
[skip ci]
…ocal The module maps (registry / fallback / fallbackByRelative / vendor) were thread_local, which is only correct because each isolate is currently pinned to a single thread. Key them by v8::Isolate* instead so they follow the owning isolate even if it's ever entered from another thread under v8::Locker per @edusperoni feedback. Per-isolate state now lives in a mutex-guarded side table and is torn down from ~Runtime via DestroyModuleStateForIsolate() while the isolate is still alive. This retires the leaky-pointer hack and also frees worker isolates' fallback/vendor maps, which were previously leaked (only the main isolate's were cleared via CleanupImportMapGlobals). Behavior-preserving: each call site binds a same-named local alias (auto& g_moduleRegistry = ModuleRegistryFor(isolate)), so resolver/loader bodies are unchanged. The two standalone helpers (RemoveModuleFromRegistry, GetLoadedModuleUrls) resolve the current isolate internally.
ada922f to
c81143d
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
NativeScript/runtime/Worker.mm (1)
228-229: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle
RunModule’s new failure return in workers.
Runtime::RunModulenow reports load/evaluation failures viafalseplusoutErrorMessage. This worker path still ignores the return value and only checksTryCatch, so non-thrown HTTP ESM/TLA failures may never reachworker.onerror.Proposed fix
- runtime->RunModule(resolvedPath); + std::string errorMessage; + bool didRun = runtime->RunModule(resolvedPath, &errorMessage); + if (!didRun && !tc.HasCaught()) { + worker->PassUncaughtExceptionFromWorkerToMain( + errorMessage.empty() ? "Worker script failed: " + resolvedPath : errorMessage, + resolvedPath, "", 1, true); + worker->Terminate(); + return isolate; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/Worker.mm` around lines 228 - 229, The worker module loading path in Worker::Start still ignores Runtime::RunModule’s new false return, so failures that only populate outErrorMessage never surface to worker.onerror. Update the RunModule call site to capture the boolean result and error message, then route that failure through the same worker error handling path used for TryCatch so both thrown and non-thrown evaluation/load errors are reported consistently.
🧹 Nitpick comments (4)
TestRunner/app/tests/HttpEsmLoaderTests.js (2)
245-249: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead code after
pending()calls.
pending()called synchronously in a spec body throws internally and halts execution immediately, so the followingdone(); return;lines never run. Harmless but misleading; can be dropped for clarity.♻️ Simplify skip guards
if (!origin) { pending("REPORT_BASEURL not set; skipping host HTTP tests"); - done(); - return; }Also applies to: 271-275, 298-302, 323-327
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TestRunner/app/tests/HttpEsmLoaderTests.js` around lines 245 - 249, The skip guards in the HTTP ESM loader specs contain dead code after synchronous pending() calls. In the relevant test blocks within HttpEsmLoaderTests.js, remove the unnecessary done(); return; lines that follow pending() so the guard reads cleanly and matches the actual control flow; apply the same simplification to the other pending() skip checks in the same test file.
242-344: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRepeated skip-guard boilerplate across canonicalization tests.
The
if (!origin) { pending(...); done(); return; }block is duplicated across all four tests in this suite. Consider extracting a small helper (e.g.requireHostOriginOrSkip(done)) alongside the existingformatError/withTimeout/getHostOriginhelpers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@TestRunner/app/tests/HttpEsmLoaderTests.js` around lines 242 - 344, The URL key canonicalization tests repeat the same host-skip guard in each case, making the suite noisy and harder to maintain. Add a small shared helper near the existing getHostOrigin, withTimeout, and formatError utilities (for example, a function like requireHostOriginOrSkip(done)) that checks for a missing origin, calls pending with the same message, then finishes the test early. Update each of the four describe("URL Key Canonicalization") specs to use that helper instead of duplicating the if (!origin) block.NativeScript/runtime/HMRSupport.mm (1)
8-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude
<memory>directly forstd::shared_ptrandstd::make_shared.This file uses
std::shared_ptrandstd::make_sharedlater, but the changed include list does not include<memory>, so it relies on transitive includes.Proposed fix
`#include` <mutex> +#include <memory> `#include` "Helpers.h"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/HMRSupport.mm` around lines 8 - 18, Add the missing direct include for <memory> in HMRSupport.mm because the file uses std::shared_ptr and std::make_shared and should not rely on transitive headers. Update the include block near the top of the file so the needed standard library types are declared explicitly, keeping the existing RuntimeConfig, Worker, and helper includes unchanged..github/workflows/npm_release.yml (1)
262-279: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUDID fallback may resolve to the wrong simulator on a multi-runtime runner.
If no device is currently booted, the fallback searches
xcrun simctl list devices 'iPhone 16 Pro'across every installed runtime and takeshead -1. On a runner image with multiple Xcode/runtime versions pre-installed, this can match a same-named simulator from an unrelated iOS runtime that never ran the tests, producing an essentially empty/irrelevantsimulator.logarchiveinstead of the one from the actual failing run — undermining the stated goal of showing "the app's console output ... before a hang" (Line 262-263 comment).♻️ Possible mitigation
Filter the fallback list by the runtime actually used for testing (e.g. match the
OS=latestruntime forenv.XCODE_VERSION), or persist the UDID resolved byxcodebuild -destinationduring the "Xcode Tests" step to a file and reuse it here instead of re-searching by name only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/npm_release.yml around lines 262 - 279, The UDID lookup in the simulator log collection step can pick the wrong device on runners with multiple runtimes because it falls back to a name-only search in xcrun simctl list devices. Update the logarchive collection logic to reuse the exact simulator used by the Xcode Tests step or filter the fallback by the same runtime/destination as the test run, then keep using that resolved UDID in the xcrun simctl spawn log collect path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@NativeScript/runtime/HMRSupport.mm`:
- Around line 806-814: Timed-out prefetch work in HMRSupport.mm can still
complete later and overwrite g_prefetchCache with stale data after
dispatch_group_wait has already returned. Add a per-prefetch generation or
cancel marker around the prefetch flow in the relevant HMRSupport functions
handling the cache write path, and check that marker immediately before storing
the fetched body. Make sure the guard is shared by the scheduled blocks in both
affected sections so only the active reload cycle can commit into
g_prefetchCache.
- Around line 36-43: `InitializeHmrDevGlobals` only defines the `globalThis`
mirror once, so repeated or re-entrant installs can leave
`globalThis.__NS_DEV__` pointing at a stale object. Update the generated
assignment in `HMRSupport.mm` so the mirror is refreshed on each install instead
of only when undefined, while still preserving the `Object.defineProperty`
behavior used for `name` and the `__NS_DEV__` global.
In `@NativeScript/runtime/Runtime.mm`:
- Around line 137-148: Update the hasUrlScheme handling in Runtime::dirname
logic so single-segment HTTP-style module URLs like http://host/main.js are
reduced to the host root for import.meta.dirname instead of keeping the full
module URL. The current lastSlash > pathStart check in the modulePath branch is
too strict; adjust the condition or split logic so the final path segment is
stripped whenever a URL path exists after the host, while still preserving the
identity for host-only or non-hierarchical schemes such as node:fs and blob:abc.
In `@NativeScript/runtime/URLImpl.cpp`:
- Around line 61-89: The cached URLSearchParams instance in URLImpl is not
refreshed when the search string is reassigned, so stale values can be returned
after setting URL.search. Update the SetSearch logic in URLImpl to either clear
_searchParams when search changes or synchronize the existing object with the
new query string, so later URL.searchParams access reflects the latest value.
- Around line 26-91: The injected blob URL setup in URLImpl.cpp is not
idempotent: repeated execution can redeclare BLOB_STORE and InternalAccessor and
can fail when redefining URL.prototype.searchParams. Update the blob_methods
script so its top-level declarations are guarded or reused on subsequent
installs, and make the searchParams property definition configurable so the
accessor can be safely reinstalled without throwing. Use the existing
URL.createObjectURL, URL.revokeObjectURL, InternalAccessor, and
Object.defineProperty(URL.prototype, 'searchParams', ...) sections as the fix
points.
---
Outside diff comments:
In `@NativeScript/runtime/Worker.mm`:
- Around line 228-229: The worker module loading path in Worker::Start still
ignores Runtime::RunModule’s new false return, so failures that only populate
outErrorMessage never surface to worker.onerror. Update the RunModule call site
to capture the boolean result and error message, then route that failure through
the same worker error handling path used for TryCatch so both thrown and
non-thrown evaluation/load errors are reported consistently.
---
Nitpick comments:
In @.github/workflows/npm_release.yml:
- Around line 262-279: The UDID lookup in the simulator log collection step can
pick the wrong device on runners with multiple runtimes because it falls back to
a name-only search in xcrun simctl list devices. Update the logarchive
collection logic to reuse the exact simulator used by the Xcode Tests step or
filter the fallback by the same runtime/destination as the test run, then keep
using that resolved UDID in the xcrun simctl spawn log collect path.
In `@NativeScript/runtime/HMRSupport.mm`:
- Around line 8-18: Add the missing direct include for <memory> in HMRSupport.mm
because the file uses std::shared_ptr and std::make_shared and should not rely
on transitive headers. Update the include block near the top of the file so the
needed standard library types are declared explicitly, keeping the existing
RuntimeConfig, Worker, and helper includes unchanged.
In `@TestRunner/app/tests/HttpEsmLoaderTests.js`:
- Around line 245-249: The skip guards in the HTTP ESM loader specs contain dead
code after synchronous pending() calls. In the relevant test blocks within
HttpEsmLoaderTests.js, remove the unnecessary done(); return; lines that follow
pending() so the guard reads cleanly and matches the actual control flow; apply
the same simplification to the other pending() skip checks in the same test
file.
- Around line 242-344: The URL key canonicalization tests repeat the same
host-skip guard in each case, making the suite noisy and harder to maintain. Add
a small shared helper near the existing getHostOrigin, withTimeout, and
formatError utilities (for example, a function like
requireHostOriginOrSkip(done)) that checks for a missing origin, calls pending
with the same message, then finishes the test early. Update each of the four
describe("URL Key Canonicalization") specs to use that helper instead of
duplicating the if (!origin) block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: db3ffc7d-0cdb-488b-a6db-465fc500c772
📒 Files selected for processing (26)
.github/scripts/sample-hung-app.sh.github/workflows/npm_release.ymlNativeScript/runtime/DevFlags.hNativeScript/runtime/DevFlags.mmNativeScript/runtime/HMRSupport.hNativeScript/runtime/HMRSupport.mmNativeScript/runtime/ModuleInternal.hNativeScript/runtime/ModuleInternal.mmNativeScript/runtime/ModuleInternalCallbacks.hNativeScript/runtime/ModuleInternalCallbacks.mmNativeScript/runtime/Runtime.hNativeScript/runtime/Runtime.mmNativeScript/runtime/URLImpl.cppNativeScript/runtime/URLImpl.hNativeScript/runtime/Worker.hNativeScript/runtime/Worker.mmTestRunner/app/Infrastructure/Jasmine/jasmine-2.0.1/boot.jsTestRunner/app/tests/HttpEsmLoaderTests.jsTestRunner/app/tests/MethodCallsTests.jsTestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjsTestRunner/app/tests/RemoteModuleSecurityTests.jsTestRunnerTests/Embassy/DefaultHTTPServer.swiftTestRunnerTests/Embassy/TCPSocket.swiftTestRunnerTests/Embassy/Transport.swiftTestRunnerTests/QUARANTINED_TESTS.mdTestRunnerTests/TestRunnerTests.swift
✅ Files skipped from review due to trivial changes (1)
- TestRunnerTests/QUARANTINED_TESTS.md
🚧 Files skipped from review as they are similar to previous changes (15)
- NativeScript/runtime/ModuleInternal.h
- .github/scripts/sample-hung-app.sh
- NativeScript/runtime/URLImpl.h
- NativeScript/runtime/Worker.h
- TestRunner/app/tests/MethodCallsTests.js
- NativeScript/runtime/Runtime.h
- TestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjs
- TestRunnerTests/Embassy/TCPSocket.swift
- TestRunner/app/tests/RemoteModuleSecurityTests.js
- TestRunnerTests/Embassy/DefaultHTTPServer.swift
- NativeScript/runtime/ModuleInternalCallbacks.h
- TestRunnerTests/Embassy/Transport.swift
- TestRunner/app/Infrastructure/Jasmine/jasmine-2.0.1/boot.js
- TestRunnerTests/TestRunnerTests.swift
- NativeScript/runtime/ModuleInternal.mm
| std::string src = | ||
| "if (typeof globalThis !== 'undefined' && typeof globalThis." + | ||
| std::string(name) + | ||
| " === 'undefined') {" | ||
| " Object.defineProperty(globalThis, '" + std::string(name) + | ||
| "', { value: this." + std::string(name) + | ||
| ", writable: true, configurable: true, enumerable: false });" | ||
| "}"; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the globalThis mirror in sync on repeated installation.
InitializeHmrDevGlobals creates a fresh __NS_DEV__ object, but this mirror only writes when globalThis.__NS_DEV__ is undefined. A re-entrant install can leave globalThis.__NS_DEV__ pointing at the old object.
Proposed fix
- "if (typeof globalThis !== 'undefined' && typeof globalThis." +
+ "if (typeof globalThis !== 'undefined') {" +
std::string(name) +
- " === 'undefined') {"
" Object.defineProperty(globalThis, '" + std::string(name) +
"', { value: this." + std::string(name) +
", writable: true, configurable: true, enumerable: false });"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@NativeScript/runtime/HMRSupport.mm` around lines 36 - 43,
`InitializeHmrDevGlobals` only defines the `globalThis` mirror once, so repeated
or re-entrant installs can leave `globalThis.__NS_DEV__` pointing at a stale
object. Update the generated assignment in `HMRSupport.mm` so the mirror is
refreshed on each install instead of only when undefined, while still preserving
the `Object.defineProperty` behavior used for `name` and the `__NS_DEV__`
global.
| if (ok && status >= 200 && status < 300 && !body.empty()) { | ||
| const size_t bodySize = body.size(); | ||
| // Overwrite unconditionally — the fresh body we just fetched is | ||
| // by definition the authoritative copy; any older cache entry is | ||
| // stale by construction (the caller has just told us so). | ||
| { | ||
| std::lock_guard<std::mutex> lock(g_prefetchMutex); | ||
| g_prefetchCache[urlCopy] = std::move(body); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Prevent timed-out prefetch waves from writing stale bodies later.
When dispatch_group_wait times out, the scheduled blocks keep running and can still write into g_prefetchCache after the caller has fallen back or started a newer reload cycle. Add a generation/cancel flag and check it immediately before caching the fetched body.
Proposed fix
struct KickstartContext {
std::mutex mutex;
std::unordered_set<std::string> visited;
+ std::atomic<bool> acceptResults{true};
std::atomic<size_t> fetchedCount{0};
std::atomic<size_t> bytes{0}; if (ok && status >= 200 && status < 300 && !body.empty()) {
const size_t bodySize = body.size();
- // Overwrite unconditionally — the fresh body we just fetched is
- // by definition the authoritative copy; any older cache entry is
- // stale by construction (the caller has just told us so).
{
std::lock_guard<std::mutex> lock(g_prefetchMutex);
- g_prefetchCache[urlCopy] = std::move(body);
+ if (ctx->acceptResults.load(std::memory_order_acquire)) {
+ g_prefetchCache[urlCopy] = std::move(body);
+ }
} const uint64_t elapsedMs = endUs > startUs ? (endUs - startUs) / 1000ull : 0ull;
+ if (timedOut != 0) {
+ ctx->acceptResults.store(false, std::memory_order_release);
+ }
const size_t fetched = ctx->fetchedCount.load(std::memory_order_relaxed);Also applies to: 883-908
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@NativeScript/runtime/HMRSupport.mm` around lines 806 - 814, Timed-out
prefetch work in HMRSupport.mm can still complete later and overwrite
g_prefetchCache with stale data after dispatch_group_wait has already returned.
Add a per-prefetch generation or cancel marker around the prefetch flow in the
relevant HMRSupport functions handling the cache write path, and check that
marker immediately before storing the fetched body. Make sure the guard is
shared by the scheduled blocks in both affected sections so only the active
reload cycle can commit into g_prefetchCache.
| } else if (hasUrlScheme(modulePath)) { | ||
| size_t schemeEnd = modulePath.find("://"); | ||
| size_t pathStart = (schemeEnd == std::string::npos) ? std::string::npos | ||
| : modulePath.find('/', schemeEnd + 3); | ||
| size_t lastSlash = modulePath.find_last_of('/'); | ||
| if (pathStart != std::string::npos && lastSlash != std::string::npos && | ||
| lastSlash > pathStart) { | ||
| dirname = modulePath.substr(0, lastSlash); | ||
| } else { | ||
| // No path beyond the host (`http://host`) or scheme without `//` | ||
| // (`node:fs`, `blob:abc`). Keep the identity intact. | ||
| dirname = modulePath; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Strip single-segment HTTP module paths for import.meta.dirname.
Line 142 requires lastSlash > pathStart, so http://host/main.js returns the full module URL instead of http://host. That contradicts the “final segment stripped” behavior and affects a common dev-server URL shape.
Proposed fix
- if (pathStart != std::string::npos && lastSlash != std::string::npos &&
- lastSlash > pathStart) {
+ if (pathStart != std::string::npos && lastSlash != std::string::npos &&
+ lastSlash >= pathStart && lastSlash + 1 < modulePath.size()) {
dirname = modulePath.substr(0, lastSlash);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } else if (hasUrlScheme(modulePath)) { | |
| size_t schemeEnd = modulePath.find("://"); | |
| size_t pathStart = (schemeEnd == std::string::npos) ? std::string::npos | |
| : modulePath.find('/', schemeEnd + 3); | |
| size_t lastSlash = modulePath.find_last_of('/'); | |
| if (pathStart != std::string::npos && lastSlash != std::string::npos && | |
| lastSlash > pathStart) { | |
| dirname = modulePath.substr(0, lastSlash); | |
| } else { | |
| // No path beyond the host (`http://host`) or scheme without `//` | |
| // (`node:fs`, `blob:abc`). Keep the identity intact. | |
| dirname = modulePath; | |
| } else if (hasUrlScheme(modulePath)) { | |
| size_t schemeEnd = modulePath.find("://"); | |
| size_t pathStart = (schemeEnd == std::string::npos) ? std::string::npos | |
| : modulePath.find('/', schemeEnd + 3); | |
| size_t lastSlash = modulePath.find_last_of('/'); | |
| if (pathStart != std::string::npos && lastSlash != std::string::npos && | |
| lastSlash >= pathStart && lastSlash + 1 < modulePath.size()) { | |
| dirname = modulePath.substr(0, lastSlash); | |
| } else { | |
| // No path beyond the host (`http://host`) or scheme without `//` | |
| // (`node:fs`, `blob:abc`). Keep the identity intact. | |
| dirname = modulePath; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@NativeScript/runtime/Runtime.mm` around lines 137 - 148, Update the
hasUrlScheme handling in Runtime::dirname logic so single-segment HTTP-style
module URLs like http://host/main.js are reduced to the host root for
import.meta.dirname instead of keeping the full module URL. The current
lastSlash > pathStart check in the modulePath branch is too strict; adjust the
condition or split logic so the final path segment is stripped whenever a URL
path exists after the host, while still preserving the identity for host-only or
non-hierarchical schemes such as node:fs and blob:abc.
| auto blob_methods = R"js( | ||
| const BLOB_STORE = new Map(); | ||
| URL.createObjectURL = function (object, options = null) { | ||
| try { | ||
| if (object instanceof Blob || object instanceof File) { | ||
| const id = NSUUID.UUID().UUIDString.toLowerCase(); | ||
| const ret = `blob:nativescript/${id}`; | ||
| BLOB_STORE.set(ret, { | ||
| blob: object, | ||
| type: object?.type, | ||
| ext: options?.ext, | ||
| }); | ||
| return ret; | ||
| } | ||
| } catch (error) { | ||
| return null; | ||
| } | ||
| return null; | ||
| }; | ||
| URL.revokeObjectURL = function (url) { | ||
| BLOB_STORE.delete(url); | ||
| }; | ||
| const InternalAccessor = class {}; | ||
| InternalAccessor.getData = function (url) { | ||
| return BLOB_STORE.get(url); | ||
| }; | ||
| // Get the text content directly from a blob URL (for HMR) | ||
| InternalAccessor.getText = async function (url) { | ||
| const data = BLOB_STORE.get(url); | ||
| if (!data || !data.blob) return null; | ||
| return await data.blob.text(); | ||
| }; | ||
| URL.InternalAccessor = InternalAccessor; | ||
| Object.defineProperty(URL.prototype, 'searchParams', { | ||
| get() { | ||
| if (this._searchParams == null) { | ||
| this._searchParams = new URLSearchParams(this.search); | ||
| Object.defineProperty(this._searchParams, '_url', { | ||
| enumerable: false, | ||
| writable: false, | ||
| value: this, | ||
| }); | ||
| this._searchParams._append = this._searchParams.append; | ||
| this._searchParams.append = function (name, value) { | ||
| this._append(name, value); | ||
| this._url.search = this.toString(); | ||
| }; | ||
| this._searchParams._delete = this._searchParams.delete; | ||
| this._searchParams.delete = function (name) { | ||
| this._delete(name); | ||
| this._url.search = this.toString(); | ||
| }; | ||
| this._searchParams._set = this._searchParams.set; | ||
| this._searchParams.set = function (name, value) { | ||
| this._set(name, value); | ||
| this._url.search = this.toString(); | ||
| }; | ||
| this._searchParams._sort = this._searchParams.sort; | ||
| this._searchParams.sort = function () { | ||
| this._sort(); | ||
| this._url.search = this.toString(); | ||
| }; | ||
| } | ||
| return this._searchParams; | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make this install script safe to run more than once.
BLOB_STORE and InternalAccessor are declared at top level, so re-executing the injected script in the same V8 context can fail on redeclaration. The searchParams accessor is also defined without configurable: true, which makes repeat installation brittle.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@NativeScript/runtime/URLImpl.cpp` around lines 26 - 91, The injected blob URL
setup in URLImpl.cpp is not idempotent: repeated execution can redeclare
BLOB_STORE and InternalAccessor and can fail when redefining
URL.prototype.searchParams. Update the blob_methods script so its top-level
declarations are guarded or reused on subsequent installs, and make the
searchParams property definition configurable so the accessor can be safely
reinstalled without throwing. Use the existing URL.createObjectURL,
URL.revokeObjectURL, InternalAccessor, and Object.defineProperty(URL.prototype,
'searchParams', ...) sections as the fix points.
| if (this._searchParams == null) { | ||
| this._searchParams = new URLSearchParams(this.search); | ||
| Object.defineProperty(this._searchParams, '_url', { | ||
| enumerable: false, | ||
| writable: false, | ||
| value: this, | ||
| }); | ||
| this._searchParams._append = this._searchParams.append; | ||
| this._searchParams.append = function (name, value) { | ||
| this._append(name, value); | ||
| this._url.search = this.toString(); | ||
| }; | ||
| this._searchParams._delete = this._searchParams.delete; | ||
| this._searchParams.delete = function (name) { | ||
| this._delete(name); | ||
| this._url.search = this.toString(); | ||
| }; | ||
| this._searchParams._set = this._searchParams.set; | ||
| this._searchParams.set = function (name, value) { | ||
| this._set(name, value); | ||
| this._url.search = this.toString(); | ||
| }; | ||
| this._searchParams._sort = this._searchParams.sort; | ||
| this._searchParams.sort = function () { | ||
| this._sort(); | ||
| this._url.search = this.toString(); | ||
| }; | ||
| } | ||
| return this._searchParams; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect URL search setter/getter implementations and any _searchParams invalidation.
fd -i 'URLImpl\.(cpp|h)$' . --exec sh -c '
echo "== $1 ==";
rg -n -C 4 "searchParams|_searchParams|Set.*Search|search\\s*=|Get.*Search" "$1"
' sh {}Repository: NativeScript/ios
Length of output: 3941
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="NativeScript/runtime/URLImpl.cpp"
echo "== relevant _searchParams/search locations =="
rg -n -C 3 "_searchParams|GetSearch|SetSearch|searchParams" "$file"
echo
echo "== SetSearch implementation =="
sed -n '516,560p' "$file"
echo
echo "== surrounding search getter/setter area =="
sed -n '502,560p' "$file"Repository: NativeScript/ios
Length of output: 6332
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all _searchParams references =="
rg -n "_searchParams" NativeScript/runtime . || true
echo
echo "== all searchParams accessor/setup references =="
rg -n "searchParams" NativeScript/runtime . || trueRepository: NativeScript/ios
Length of output: 7249
Keep searchParams in sync when search is assigned. searchParams is cached once, but SetSearch never clears or refreshes _searchParams, so after url.searchParams has been accessed once, url.search = "?b=2"; url.searchParams.get("b") can read stale data. Clear the cache or update the existing params object in the setter.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@NativeScript/runtime/URLImpl.cpp` around lines 61 - 89, The cached
URLSearchParams instance in URLImpl is not refreshed when the search string is
reassigned, so stale values can be returned after setting URL.search. Update the
SetSearch logic in URLImpl to either clear _searchParams when search changes or
synchronize the existing object with the new query string, so later
URL.searchParams access reflects the latest value.
…ract The runtime explicitly does not implement HMR policy. import.meta.hot, the hot-data/accept/dispose/prune registries, and dev-session state move to the JS HMR clients (eg, @nativescript/vite); native keeps only the sync HTTP module fetch, prewarm cache + list-mode kickstart, eviction plumbing, and the dev-boot-complete signal. Dev helpers are consolidated under __NS_DEV__
c81143d to
e48b2ca
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
NativeScript/runtime/ModuleInternal.mm (1)
1031-1040: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate HTTP loader failures instead of returning an empty namespace
LoadHttpModuleForUrlalready throws on fetch/compile errors, but this debug branch turns that intoLocal<Value>(), soRunModulefalls back to the generic empty-namespace message and drops the real cause. Throw here forisHttpModulesooutErrorMessagecarries the loader error.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/ModuleInternal.mm` around lines 1031 - 1040, The HTTP module load path in ModuleInternal.mm is swallowing the real loader failure in the isHttpModule branch by returning an empty Local<Value>() when LoadHttpModuleForUrl fails. Update the RunModule/module compilation flow so that, instead of returning empty in the RuntimeConfig.IsDebug branch, it propagates the exception from LoadHttpModuleForUrl (or throws a NativeScriptException with the loader error) and lets outErrorMessage capture that cause. Keep the existing logPhase("compile", "fail", "http-loader") but ensure the failure exits via an error path, not a fallback namespace return.
♻️ Duplicate comments (1)
NativeScript/runtime/ModuleInternal.mm (1)
234-256: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winWorker
.mjsESM failures are still swallowed in debug.The CJS
require()path was updated to gate on!cache->isWorker(Line 300) and rethrows for workers (Lines 348-350), but the ESM branch here still gates only on!isHttpModule. For a.mjsworker entry, a load failure returnstrue(Line 240) or an empty namespace returnstrue(Line 256), soWorker.mm'sTryCatchnever observes the failure andworker.onerrornever fires. This is the same concern raised previously and marked addressed — the fix appears to have landed only on the CJS path.Proposed direction
- if (RuntimeConfig.IsDebug && !isHttpModule) { + if (RuntimeConfig.IsDebug && !isHttpModule && !cache->isWorker) { Log(@"***** JavaScript exception occurred - detailed stack trace follows *****"); ... return true; // avoid termination in debug } else { SetOutErrorMessage(outErrorMessage, ex.getMessage()); + if (cache->isWorker && /* pending V8 exception available */) { + // rethrow so Worker.mm TryCatch routes to worker.onerror + } return false; } @@ - if (RuntimeConfig.IsDebug && !isHttpModule) { + if (RuntimeConfig.IsDebug && !isHttpModule && !cache->isWorker) { Log(@"Debug mode - ES module returned empty namespace, but telling iOS it succeeded"); return true;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@NativeScript/runtime/ModuleInternal.mm` around lines 234 - 256, The ESM worker load path in ModuleInternal.mm still swallows failures in debug because it only checks RuntimeConfig.IsDebug and !isHttpModule, so .mjs worker entry errors return true instead of propagating. Update the ES module handling around the load-failure and empty-namespace branches to also exclude worker contexts, using the same worker-aware gating already applied in the require()/cache path, so Worker.mm’s TryCatch can observe the exception and fire worker.onerror.
🧹 Nitpick comments (1)
.github/workflows/npm_release.yml (1)
171-176: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUpdate the runtime-suite note to match the actual pin.
XCODE_VERSIONis^15.0, so the Xcode 26/iOS 26 comment is stale and misleading.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/npm_release.yml around lines 171 - 176, The runtime-suite note is stale and does not match the pinned Xcode version. Update the comment near the workflow’s macOS runner and XCODE_VERSION pin to describe the actual Xcode 15 / iOS 15 runtime instead of Xcode 26 / iOS 26, keeping the note consistent with the deterministic pinning rationale in the same block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@NativeScript/runtime/ModuleInternal.mm`:
- Around line 1031-1040: The HTTP module load path in ModuleInternal.mm is
swallowing the real loader failure in the isHttpModule branch by returning an
empty Local<Value>() when LoadHttpModuleForUrl fails. Update the
RunModule/module compilation flow so that, instead of returning empty in the
RuntimeConfig.IsDebug branch, it propagates the exception from
LoadHttpModuleForUrl (or throws a NativeScriptException with the loader error)
and lets outErrorMessage capture that cause. Keep the existing
logPhase("compile", "fail", "http-loader") but ensure the failure exits via an
error path, not a fallback namespace return.
---
Duplicate comments:
In `@NativeScript/runtime/ModuleInternal.mm`:
- Around line 234-256: The ESM worker load path in ModuleInternal.mm still
swallows failures in debug because it only checks RuntimeConfig.IsDebug and
!isHttpModule, so .mjs worker entry errors return true instead of propagating.
Update the ES module handling around the load-failure and empty-namespace
branches to also exclude worker contexts, using the same worker-aware gating
already applied in the require()/cache path, so Worker.mm’s TryCatch can observe
the exception and fire worker.onerror.
---
Nitpick comments:
In @.github/workflows/npm_release.yml:
- Around line 171-176: The runtime-suite note is stale and does not match the
pinned Xcode version. Update the comment near the workflow’s macOS runner and
XCODE_VERSION pin to describe the actual Xcode 15 / iOS 15 runtime instead of
Xcode 26 / iOS 26, keeping the note consistent with the deterministic pinning
rationale in the same block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 72cb5ffd-ae59-44eb-b7be-58246e617135
📒 Files selected for processing (14)
.github/workflows/npm_release.ymlNativeScript/runtime/DevFlags.hNativeScript/runtime/DevFlags.mmNativeScript/runtime/HMRSupport.hNativeScript/runtime/HMRSupport.mmNativeScript/runtime/ModuleInternal.mmNativeScript/runtime/ModuleInternalCallbacks.mmNativeScript/runtime/Runtime.mmNativeScript/runtime/URLImpl.hNativeScript/runtime/Worker.hNativeScript/runtime/Worker.mmTestRunner/app/tests/HttpEsmLoaderTests.jsTestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjsTestRunnerTests/TestRunnerTests.swift
💤 Files with no reviewable changes (6)
- NativeScript/runtime/URLImpl.h
- NativeScript/runtime/DevFlags.h
- NativeScript/runtime/Worker.mm
- TestRunnerTests/TestRunnerTests.swift
- NativeScript/runtime/HMRSupport.mm
- NativeScript/runtime/DevFlags.mm
🚧 Files skipped from review as they are similar to previous changes (5)
- NativeScript/runtime/Worker.h
- TestRunner/app/tests/NodeBuiltinsAndOptionalModulesTests.mjs
- TestRunner/app/tests/HttpEsmLoaderTests.js
- NativeScript/runtime/HMRSupport.h
- NativeScript/runtime/Runtime.mm
Framework-agnostic hot module replacement on iOS with native ES modules: the device fetches modules over HTTP from the Vite dev server and applies hot updates without restarting the process.
The runtime's entire dev surface is one namespace object,
globalThis.__NS_DEV__, carrying six primitives, each traceable to a V8-embedder or OS constraint:configureRuntime(config)ResolveModuleCallbackinvalidateModules(urls)v8::Modulerecords + prewarm cache + arms a CFNetwork cache-bust noncekickstartPrefetch(urls)getLoadedModuleUrls()setDevBootComplete(bool)terminateAllWorkers()Caches::Workers(runtime-owned, not JS-enumerable); catch-all for worker spawns no per-module dispose can observe. Main isolate onlyEvery HMR policy decision, boot orchestration,
import.meta.hot, full reload, CSS apply, the WebSocket protocol is JS in@nativescript/vite. No native dev-session state machine, no reload orchestration, no hot-callback registries exist in the runtime, and on-device tests pin both the present members and the absent globals.Module identity & freshness
Module identity is the canonical URL: the server emits exactly one URL per module and never varies it for freshness (this closes the realm-split /
Cannot redefine propertycrash class). Canonicalization survives only to absorb externally-caused variance (Vite's?v=/?import/?t=markers,file://http://wrapping). Freshness is explicit three-layer eviction: V8 registry > kickstart prewarm cache > a one-shot__ns_dev_noncethat defeats CFNetwork's cache (observed serving stale bodies on iOS 18+/26 Sim despiteno-storeand a zero-capacityNSURLCache).Additional
ModuleInternalCallbacks.mm): HTTP(S) URLs end-to-end (resolve, fetch, dynamic import) and.jsonimports compiled into synthetic ES modules.v8::Isolate*(not thread-locals); worker teardown no longer wipes main-isolate dev state; worker entry-script errors propagate toworker.onerror.